packages/loot-core/src/server/util/budget-name.ts (1)

6-9: Approve changes with a minor suggestion for type safety.

The addition of the initialName parameter with a default value enhances the function's flexibility while maintaining backward compatibility. This change aligns well with the PR objectives, particularly in supporting the new duplicate feature.

Consider adding type annotations to improve type safety:

export async function uniqueFileName(
  existingFiles: { name: string }[],
  initialName: string = 'My Finances'
): Promise<string> {
  // ... rest of the function
}

This change would make the function's interface more explicit and catch potential type-related issues early.

packages/loot-core/src/server/backups.ts (1)

138-138: Add JSDoc comment for the removeAllBackups function.

To improve code documentation and clarify the function's purpose, consider adding a JSDoc comment above the removeAllBackups function.

Here's a suggested JSDoc comment:

/**
 * Removes all backup files associated with the specified budget ID.
 * This function is typically used when deleting a budget to ensure all related backups are also removed.
 * @param {string} id - The ID of the budget whose backups should be removed.
 * @returns {Promise<boolean>} A promise that resolves to true if all backups were successfully removed, false otherwise.
 */
export async function removeAllBackups(id: string): Promise<boolean> {
  // ... function implementation ...
}

This comment provides clear information about the function's purpose, parameters, and return value, which will be helpful for other developers using or maintaining this code.

packages/loot-core/src/client/actions/budgets.ts (1)

151-192: Good implementation with room for improvements

The duplicateBudget function is well-structured and follows the existing patterns in the file. However, there are a few areas that could be improved:

1. Error handling: Consider adding explicit error handling for the 'duplicate-budget' action to manage potential failures gracefully.

2. Loading state: When managePage is true, the loading state is not reset. Consider adding a loading state for this case as well.

3. CloudSync parameter: The purpose of the cloudSync parameter is not clear from this function. Consider adding a comment to explain its usage or impact.

Here's a suggested improvement to address these points:

export function duplicateBudget({
  id,
  cloudId,
  oldName,
  newName,
  managePage,
  cloudSync,
}: {
  id?: string;
  cloudId?: string;
  oldName: string;
  newName: string;
  managePage?: boolean;
  cloudSync?: boolean; // Add a comment explaining the purpose of this parameter
}) {
  return async (dispatch: Dispatch) => {
    try {
      if (!managePage) {
        await dispatch(closeBudget());
        await dispatch(
          setAppState({
            loadingText:
              t('Duplicating:  ') + oldName + t('  --  to:  ') + newName,
          }),
        );
      } else {
        dispatch(setAppState({ loadingText: t('Duplicating budget...') }));
      }

      const newId = await send('duplicate-budget', {
        id,
        cloudId,
        newName,
        cloudSync,
      });

      dispatch(closeModal());

      if (managePage) {
        await dispatch(loadAllFiles());
        await dispatch(loadPrefs());
      } else {
        await dispatch(loadBudget(newId));
      }
    } catch (error) {
      // Handle error appropriately
      console.error('Error duplicating budget:', error);
      dispatch(setAppState({ loadingText: null }));
      // Consider showing an error modal or alert to the user
    } finally {
      dispatch(setAppState({ loadingText: null }));
    }
  };
}

This refactored version adds error handling, manages loading states for both cases, and suggests adding a comment for the cloudSync parameter.

packages/loot-core/src/client/state-types/modals.d.ts (2)

52-52: LGTM! Consider adding JSDoc comment for clarity.

The addition of the budgetId property to the 'load-backup' modal type is a good improvement. It allows for more flexibility in the backup loading process, which aligns with the PR objectives.

Consider adding a JSDoc comment to explain the purpose of the budgetId property and when it might be undefined. For example:

'load-backup': {
  /**
   * The ID of the budget to load a backup for.
   * If undefined, it may indicate loading a backup for the current budget or a user-selected budget.
   */
  budgetId: string | undefined
};

---

81-82: LGTM! Consider adding JSDoc comment for better documentation.

The addition of the 'duplicate-budget' modal type is well-aligned with the PR objectives. It provides the necessary properties for duplicating a budget file.

To improve clarity and maintainability, consider adding a JSDoc comment to explain the purpose of each property. For example:

'duplicate-budget': {
  /** The budget file to be duplicated */
  file: File;
  /** 
   * Indicates whether the duplication is initiated from a management page.
   * This may affect the behavior or UI of the duplication process.
   */
  managePage?: boolean;
};

packages/loot-core/src/types/server-handlers.d.ts (1)

319-324: LGTM! Consider adding JSDoc comments for clarity.

The new duplicate-budget method aligns well with the PR objectives. The method signature allows for duplicating both local and cloud-synced budgets, which is great for flexibility.

Consider adding JSDoc comments to clarify the purpose of each parameter and the return value. For example:

/**
 * Duplicates a budget file.
 * @param {Object} arg - The arguments for duplicating a budget.
 * @param {string} [arg.id] - The ID of the local budget to duplicate.
 * @param {string} [arg.cloudId] - The ID of the cloud-synced budget to duplicate.
 * @param {string} arg.newName - The name for the duplicated budget.
 * @param {boolean} [arg.cloudSync] - Whether to sync the duplicated budget to the cloud.
 * @returns {Promise<string>} The ID of the newly created budget.
 */
'duplicate-budget': (arg: {
  id?: string;
  cloudId?: string;
  newName: string;
  cloudSync?: boolean;
}) => Promise<string>;

This addition would improve the code's self-documentation and make it easier for other developers to understand and use the method correctly.

packages/desktop-client/src/components/manager/BudgetList.tsx (2)

Line range hint 67-92: LGTM with a suggestion for improved type safety

The changes to the FileMenu component look good. The dynamic generation of menu items based on the presence of the onDuplicate prop is a clean approach.

Consider using a more specific type for the onMenuSelect function parameter:

function onMenuSelect(type: 'delete' | 'duplicate') {
  // ...
}

This will provide better type safety and autocomplete support.

---

Line range hint 190-265: LGTM with suggestions for improved type safety and clarity

The addition of the onDuplicate functionality to the FileItem component is implemented correctly. The condition 'id' in file ensures that only local files can be duplicated, which is a good safeguard.

Consider the following improvements:

1. Use type guards to improve type safety:

function isLocalFile(file: File): file is LocalFile {
  return 'id' in file;
}

// In the JSX:
onDuplicate={isLocalFile(file) ? () => onDuplicate(file) : undefined}

2. Add a comment explaining why only files with 'id' can be duplicated:

// Only local files (with 'id') can be duplicated
onDuplicate={isLocalFile(file) ? () => onDuplicate(file) : undefined}

These changes will enhance type safety and code clarity.

packages/desktop-client/src/components/modals/manager/DuplicateFileModal.tsx (1)

90-93: Adjust the style to align with the label in the form error message.

The marginLeft value in the FormError component may not align correctly with the form field label, especially if the label width changes. Consider using the same labelWidth value for consistency.

Apply this diff to use the same labelWidth value:

-    <FormError style={{ marginLeft: 75, color: theme.warningText }}>
+    <FormError style={{ marginLeft: 150, color: theme.warningText }}>

Alternatively, you can define a constant for labelWidth and reuse it.

 const labelWidth = 150;

 // ...

 <InlineField label={t('New Budget Name')} width="100%" labelWidth={labelWidth}>
   {/* ... */}
 </InlineField>

 // ...

 <FormError style={{ marginLeft: labelWidth, color: theme.warningText }}>
   {nameError}
 </FormError>

packages/desktop-client/src/components/sidebar/Sidebar.tsx (1)

Line range hint 150-209: Consider separating concerns within EditableBudgetName

The EditableBudgetName function currently handles displaying the budget name, editing the name, and managing the menu state. To improve code organization and maintainability, consider splitting this function into separate components:

• BudgetNameDisplay: Responsible for displaying the budget name.
• EditBudgetName: Manages the editing state and logic.
• BudgetNameMenu: Handles the menu actions and state.

This separation aligns with previous practices in the project and makes each component focus on a single responsibility.

packages/loot-core/src/server/backups.web.ts (3)

81-103: Add type annotations for function parameters

The updateBackups function lacks type annotations for its backups parameter. Adding type annotations improves code readability and type safety.

- export async function updateBackups(backups) {
+ export async function updateBackups(backups: BackupWithDate[]) {

---

15-15: Specify the type for serviceInterval

Currently, serviceInterval is initialized to null without a specific type, which may lead to type inconsistencies. Explicitly specifying its type enhances code clarity and type safety.

- let serviceInterval = null;
+ let serviceInterval: NodeJS.Timeout | null = null;

---

115-117: Ensure uniqueness of backup filenames

While the current implementation uses milliseconds to ensure unique filenames, there is still a slight chance of collisions if backups are created rapidly. Consider adding a unique identifier or UUID to the backup filenames.

- const backupId = `${currentTime.toISOString().replace(/[-:.]/g, '')}-db.backup.sqlite`;
+ const uniqueId = Math.random().toString(36).substr(2, 9);
+ const backupId = `${dateFns.format(currentTime, 'yyyyMMddHHmmssSSS')}-${uniqueId}-db.backup.sqlite`;

packages/loot-core/src/platform/server/fs/index.web.ts (2)

91-93: Use TextDecoder for efficient decoding of ArrayBuffers

Instead of manually converting ArrayBuffers to strings using String.fromCharCode and Uint16Array, leverage the TextDecoder API for better performance and readability.

Apply this change:

if (opts?.encoding === 'utf8' && ArrayBuffer.isView(item.contents)) {
-  return String.fromCharCode.apply(
-    null,
-    new Uint16Array(item.contents.buffer),
-  );
+  const decoder = new TextDecoder('utf-8');
+  return decoder.decode(item.contents);
}

---

Line range hint 113-149: Ensure consistent error handling in _writeFile function

The _writeFile function always returns true, even when exceptions may occur during execution. It's recommended to allow exceptions to propagate or handle them appropriately, ensuring consistent return behavior.

Consider removing the return true; statement and letting the function implicitly return void, or handle errors and return false when exceptions occur.

packages/loot-core/src/server/main.ts (1)

1810-1882: Add unit tests for duplicate-budget handler

To ensure the new duplicate-budget functionality works as intended across different scenarios, consider adding unit tests. Test cases could include duplicating budgets with unique names, handling name collisions, error handling during file operations, and verifying cloud synchronization behavior.

Configuration used: CodeRabbit UI
Review profile: CHILL

packages/desktop-client/src/components/sidebar/Sidebar.tsx (1)

216-216: Consider adding file size warning for backups

Given the known issues with large files mentioned in the PR objectives, consider adding a warning or size indicator when the backup option is selected for large budget files.

-    { name: 'backups', text: t('Backups') },
+    {
+      name: 'backups',
+      text: t('Backups'),
+      disabled: budgetFile && isLargeFile(budgetFile),
+      tooltip: t('Backup not available for large files')
+    },

packages/desktop-client/src/components/manager/BudgetList.tsx (1)

Line range hint 190-265: Consider making onDuplicate prop optional

The onDuplicate prop is marked as required in the type definition but used conditionally in the component. Consider making it optional to better reflect its usage:

-  onDuplicate: (file: File) => void;
+  onDuplicate?: (file: File) => void;

Configuration used: CodeRabbit UI
Review profile: CHILL

Learnt from: tlesicka
PR: actualbudget/actual#3593
File: packages/desktop-client/src/components/sidebar/Sidebar.tsx:112-116
Timestamp: 2024-10-10T02:29:05.655Z
Learning: In `packages/desktop-client/src/components/sidebar/BudgetName.tsx`, the `BudgetName` component consists of three parts: `BudgetName`, `EditBudgetName`, and the Menu. Keeping `EditBudgetName` as a separate component helps maintain cleaner code by separating concerns.

packages/desktop-client/src/components/sidebar/BudgetName.tsx (2)

67-74: Consider adding null check for allFiles selector.

The implementation looks good, but consider adding a null check or default value in the selector to prevent potential runtime issues:

-const allFiles = useSelector(state => state.budgets.allFiles || []);
+const allFiles = useSelector(state => state.budgets?.allFiles ?? []);

The type guard function is well-implemented and improves type safety.

---

87-91: Consider adding loading indicator for backup operations.

Based on the PR comments from youngcw, users might benefit from a loading indicator during backup operations. Consider:

1. Adding a loading state when dispatching the 'load-backup' modal
2. Disabling the menu item while the operation is in progress

This would address the reported UX issue where users were uncertain about the backup process status.

Example implementation:

const [isLoading, setIsLoading] = useState(false);

async function onMenuSelect(type: string) {
  setMenuOpen(false);

  switch (type) {
    case 'backups':
      if (budgetId) {
        setIsLoading(true);
        try {
          await dispatch(replaceModal('load-backup', { budgetId }));
        } finally {
          setIsLoading(false);
        }
      }
      break;
    // ... other cases
  }
}

Also applies to: 104-104

Configuration used: CodeRabbit UI
Review profile: CHILL

Learnt from: tlesicka
PR: actualbudget/actual#3593
File: packages/desktop-client/src/components/sidebar/Sidebar.tsx:112-116
Timestamp: 2024-10-10T02:29:05.655Z
Learning: In `packages/desktop-client/src/components/sidebar/BudgetName.tsx`, the `BudgetName` component consists of three parts: `BudgetName`, `EditBudgetName`, and the Menu. Keeping `EditBudgetName` as a separate component helps maintain cleaner code by separating concerns.

#!/bin/bash
# Search for other instances where remote files might be handled differently
rg -A 3 "file\.state.*remote" --type typescript

#!/bin/bash
# Search for patterns related to remote files handling
rg -A 3 "isRemoteFile|isNonRemoteFile" 

# Search for file state related patterns
rg -A 3 "file\.state"

# Look for the implementation of isNonRemoteFile function
ast-grep --pattern 'function isNonRemoteFile($_) { $$$ }'

packages/loot-core/src/server/main.ts (2)

1983-1991: Consider enhancing cloud sync error handling

While the current implementation allows offline file creation by ignoring upload errors, it could be more informative to the user about sync status.

Consider adding more detailed error logging and user feedback:

 if (cloudSync) {
   try {
     await cloudStorage.upload();
   } catch (error) {
     console.warn('Failed to sync duplicated budget to cloud:', error);
+    // Log detailed error information
+    captureBreadcrumb({
+      message: 'Budget duplication cloud sync failed',
+      category: 'sync',
+      data: { error: error.message }
+    });
     // Ignore any errors uploading. If they are offline they should
     // still be able to create files.
   }
 }

---

1935-1950: Consider using a whitelist approach for metadata cleanup

The current blacklist approach for metadata cleanup is less maintainable as new properties might be added in the future.

Consider using a whitelist approach:

-  [
-    'cloudFileId',
-    'groupId',
-    'lastUploaded',
-    'encryptKeyId',
-    'lastSyncedTimestamp',
-  ].forEach(item => {
-    if (metadata[item]) delete metadata[item];
-  });
+  const newMetadata = {
+    id: newId,
+    budgetName: budgetName,
+    // Add other properties that should be preserved
+    created: metadata.created,
+    budgetType: metadata.budgetType,
+    // ... other essential properties
+  };

Configuration used: CodeRabbit UI
Review profile: CHILL

Learnt from: tlesicka
PR: actualbudget/actual#3689
File: packages/loot-core/src/server/main.ts:1800-1805
Timestamp: 2024-11-10T16:45:25.627Z
Learning: Paths are not fully defined on initialization; they become fully defined when a budget is loaded. Therefore, loading and closing the budget before deleting is necessary to ensure that paths are properly resolved.

Learnt from: tlesicka
PR: actualbudget/actual#3689
File: packages/loot-core/src/server/main.ts:1873-1877
Timestamp: 2024-11-10T16:45:25.627Z
Learning: In the `duplicate-budget` function in `packages/loot-core/src/server/main.ts`, when `loadBudget` returns an object with an `error` property where `error` is a string, returning `error` directly is consistent with the function's declared return type `Promise<string>`.

Learnt from: tlesicka
PR: actualbudget/actual#3689
File: packages/loot-core/src/server/main.ts:1807-1809
Timestamp: 2024-11-10T16:45:25.627Z
Learning: In the function `delete-budget` in `packages/loot-core/src/server/main.ts`, the function `removeAllBackups(id)` handles its own errors internally, so additional error handling when calling it is unnecessary.